Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🥅 — Catch MaxInstancesExceededException on calendar events #47924

Merged
merged 1 commit into from
Sep 16, 2024
Merged

🥅 — Catch MaxInstancesExceededException on calendar events #47924

merged 1 commit into from
Sep 16, 2024

Conversation

ldidry
Copy link
Contributor

@ldidry ldidry commented Sep 12, 2024

Summary

When an event is recurring and has a huge number (more than 3500) of occurrences, you couldn’t see the calendar in web Interface.

Generally, you have this sort of event when using recurrence option and no end condition (see #29896).

The fetching request got this as response:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\VObject\Recur\MaxInstancesExceededException</s:exception>
  <s:message>Recurring events are only allowed to generate 3500</s:message>
</d:error>

Catching the Sabre\VObject\Recur\MaxInstancesExceededException allows to see the calendar.

Similar to #43647, #34442 and #29896

Checklist

/backport to stable28
/backport to stable29
/backport to stable30

miaulalala
miaulalala approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your PR!

ChristophWurst
ChristophWurst approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

This is best reviewed as https://github.com/nextcloud/server/pull/47924/files?w=1

The admin can't do anything about this error. Lowering the log level to warning could make sense.

@ChristophWurst ChristophWurst added the 4. to release Ready to be released and/or waiting for tests to finish label Sep 12, 2024
tcitworld
tcitworld approved these changes Sep 12, 2024
View reviewed changes
apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
@tcitworld
Copy link
Member

The admin can't do anything about this error.

Well, one can add UNTIL parameters in the calendar data. But yeah, warning might be better.

@tcitworld
Copy link
Member

/backport to stable28

@tcitworld
Copy link
Member

/backport to stable29

@tcitworld
Copy link
Member

/backport to stable30

@ldidry ldidry force-pushed the catch-MaxInstancesExceededException branch 2 times, most recently from 54f40d6 to 8580389 Compare September 12, 2024 14:56
@ldidry
Copy link
Contributor Author

ldidry commented Sep 12, 2024

I changed the log level to warning, as suggested.

@susnux susnux added the bug label Sep 12, 2024
@susnux
Copy link
Contributor

susnux commented Sep 12, 2024

@ldidry could you adjust your commit message to follow conventional commits?
(something like: fix(dav): catch `MaxInstancesExceededException` on calendar events )

@ldidry ldidry force-pushed the catch-MaxInstancesExceededException branch from 8580389 to c4d9b29 Compare September 16, 2024 07:41
@ldidry
Copy link
Contributor Author

ldidry commented Sep 16, 2024

Sure. Done.

@miaulalala miaulalala mentioned this pull request Sep 16, 2024
Open
@miaulalala
Copy link
Contributor

Last issue @ldidry - can you please run composer cs:fix so that the linter is happy?

You can ignore cypress and the performance tests.

@ldidry ldidry force-pushed the catch-MaxInstancesExceededException branch from c4d9b29 to bd21b3f Compare September 16, 2024 10:57
@ldidry
Copy link
Contributor Author

ldidry commented Sep 16, 2024

Done

@ldidry ldidry force-pushed the catch-MaxInstancesExceededException branch from bd21b3f to 615f0b2 Compare September 16, 2024 10:57
@miaulalala
Copy link
Contributor

@skjnldsv can you force merge? Cypress and Perf is not running bc fork

@miaulalala miaulalala mentioned this pull request Sep 16, 2024
Open
@nickvergessen nickvergessen merged commit cfed24c into nextcloud:master Sep 16, 2024
161 of 164 checks passed
@welcome Welcome
Copy link

welcome bot commented Sep 16, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@backportbot backportbot bot mentioned this pull request Sep 16, 2024
Merged
@tcitworld
Copy link
Member

/backport to stable29

@tcitworld
Copy link
Member

/backport to stable30

This was referenced Sep 16, 2024
Merged
Merged
@github-actions GitHub Actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miaulalala miaulalala miaulalala approved these changes

@tcitworld tcitworld tcitworld approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feedback-requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ldidry @tcitworld @susnux @miaulalala @ChristophWurst @nickvergessen